Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1)

crates/starknet_api/src/contract_class.rs line 106 at r2 (raw file):

        }
    }
}

copied from blockifier::execution::contract_class::ClassInfo to be used through staknet_api executable_tx through AccountTransaction in following PR (1688)

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

crates/starknet_api/src/contract_class.rs line 106 at r2 (raw file):

#1688 (add a HashTag for PR number to become a link :) )

crates/starknet_api/src/contract_class.rs line 100 at r4 (raw file):

            })
        }
    }

Damn, do I hate this new method... (This is not a practical comment - just sharing).
It can only return an error if we have an internal error.

Out of scope for this PR - just personal grudges. And the upside is that with the old new method deleted - the refactor I planned can take effect.

Code quote:

    pub fn new(
        contract_class: &ContractClass,
        sierra_program_length: usize,
        abi_length: usize,
    ) -> Result<Self, StarknetApiError> {
        let (contract_class_version, condition) = match contract_class {
            ContractClass::V0(_) => (0, sierra_program_length == 0),
            ContractClass::V1(_) => (1, sierra_program_length > 0),
        };

        if condition {
            Ok(Self { contract_class: contract_class.clone(), sierra_program_length, abi_length })
        } else {
            Err(StarknetApiError::ContractClassVersionSierraProgramLengthMismatch {
                contract_class_version,
                sierra_program_length,
            })
        }
    }

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @meship-starkware, and @noaov1)

crates/starknet_api/src/contract_class.rs line 106 at r2 (raw file):

thanks!

crates/starknet_api/src/contract_class.rs line 48 at r4 (raw file):

/// and other parameters derived from the original declare transaction required for billing.
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct ClassInfo {

Done.

crates/starknet_api/src/contract_class.rs line 100 at r4 (raw file):

I might ask you about it in person out of curiosity, for me to better understand your hate :)

Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @meship-starkware, and @noaov1)

crates/starknet_api/src/core.rs line 468 at r10 (raw file):

Would you change the name of the constant or just add the comment as you did?

As there were other constants along this file, I wasn't sure where best to place. I moved it just below the 'use'... should it be elsewhere?

crates/blockifier/src/fee/eth_gas_constants.rs line 4 at r10 (raw file):

pub const GAS_PER_MEMORY_ZERO_BYTE: usize = 4;
pub const GAS_PER_MEMORY_BYTE: usize = 16;
pub const WORD_WIDTH: usize = 32; //TODO(AvivG): use starknet_api::core::WORD_WIDTH instead.aig add -p

Done.

Reviewed 1 of 2 files at r11.
Reviewable status: 8 of 9 files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

crates/starknet_api/src/core.rs line 468 at r10 (raw file):

I think this is fine for now.

Maybe we want a starknet_api::constants.rs - but I think this is out of scope.

Reviewed 1 of 2 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

crates/blockifier/src/fee/eth_gas_constants.rs line 4 at r10 (raw file):

1. Great.
2. Non-blocking - how hard is it to follow through with this to-do in this PR? How many files are affected?

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

crates/blockifier/src/fee/eth_gas_constants.rs line 4 at r10 (raw file):

there are only 4 files WORD_WIDTH appears in. I still prefer to handle this in a separate PR, will link in this thread when exists

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)